-
Notifications
You must be signed in to change notification settings - Fork 412
[Dashboards] Fix type error in unitScale #3238
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Dashboards] Fix type error in unitScale #3238
Conversation
| unitScale := map[string]interface{}{} | ||
| unitScale["unit_name"] = []map[string]interface{}{{"unit_name": v.UnitName}} | ||
| unitScale := map[string]interface{}{"unit_name": v.UnitName} | ||
| m["unit_scale"] = []map[string]interface{}{unitScale} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The schema for this is:
"unit_scale": &schema.Schema{
Description: "",
Type: schema.TypeList,
MinItems: 0,
MaxItems: 1,
Optional: true,
Elem: &schema.Resource{
Schema: map[string]*schema.Schema{
"unit_name": &schema.Schema{
Description: "",
Type: schema.TypeString,
Required: true,
},
},
},
},
so m["unit_scale"] is supposed to contain a list of map[string]string if I am reading this right, not another map containing "unit_name" as a key
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The old code appears to be the cause of the error per the CI output:
level=fatal msg="Execution failed" error="error: error generating plan: exit status 1\n\nError: widget.1.group_definition.0.widget.0.query_value_definition.0.request.0.formula.0.number_format.0.unit_scale.0.unit_name: '' expected type 'string', got unconvertible type '[]map[string]interface {}', value: '[map[unit_name:0xc0009716e0]]'\n\n with datadog_dashboard.maze_api_dashboard,\n on dashboard.tf line 1, in resource \"datadog_dashboard\" \"maze_api_dashboard\":\n 1: resource \"datadog_dashboard\" \"maze_api_dashboard\" {\n\n code: INVALID_ARGUMENT origin: terraform-invocation, action: plan, cause: USER, resource: \n"
aka it is saying that the first element of unit_scale had a []map[string]interface {}' at its unit_name key and not just a string
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure why the old code was working until Wednesday though this probably has something to do with the new release that day.
| } | ||
|
|
||
| func buildNumberFormatFormulaSchema(terraformStyle map[string]interface{}) *datadogV1.WidgetNumberFormat { | ||
| func buildDatadogNumberFormatFormulaSchema(terraformStyle map[string]interface{}) *datadogV1.WidgetNumberFormat { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The buildDatadog functions are on the write path (turning HCL to Datadog schema), renamed this for clarity.
| } | ||
| } | ||
| } | ||
| if v, ok := terraformStyle["unit_scale"].([]interface{}); ok && len(v) > 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't able to actually create a dashboard with this field with unitScale until adding this too. Uses the same schema as below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this makes sense to me - is there any risk of this affecting existing terraforms? I would assume not since the comment makes it sound like this was working until recently?
I actually don't think this field was ever working with terraform and that this field only made its way into the dashboard because it was added via in the UI (it was not exclusively terraform-managed). This seems like a relatively simple change though and I don't think it would cause any issues, especially since it is basically just fixing broken code we have already (that implements a field that has been in our public API spec since February) and also adding the fix to the read path, not just the write. |
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
|
Adds
unitScaleto read and write paths.I tested this by creating a
test.tfinexamplesthat usesunit_scale:On the
masterbranch,terraform applysuccessfully creates a dashboard but omitsunit_scale:If I check out this branch and try again, this time it actually works:
If I check out master again, this time
terraform planfails, reproducing the original error aboutunit_name: